src: add cli option to exclude env vars on dr #55697

RafaelGSS · 2024-11-02T21:13:47Z

This PR introduces a new cli flag --report-exclude-env to avoid writing env vars on diagnostic reports.

Some context: https://x.com/arthurfiorette/status/1848372069198614664

lib/internal/process/report.js

src/node_report_module.cc

richardlau · 2024-11-02T22:09:48Z

For consistency with --report-exclude-network I'd prefer if this was --report-exclude-env.

RafaelGSS · 2024-11-05T19:56:24Z

PTAL @richardlau @anonrig

codecov · 2024-11-05T21:11:19Z

Codecov Report

Attention: Patch coverage is 88.70968% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (5ce3d10) to head (fed0381).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_report_module.cc	61.53%	5 Missing ⚠️
lib/internal/process/report.js	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55697      +/-   ##
==========================================
- Coverage   88.44%   88.40%   -0.05%     
==========================================
  Files         654      654              
  Lines      187720   187808      +88     
  Branches    36143    36139       -4     
==========================================
+ Hits       166022   166023       +1     
- Misses      14941    15020      +79     
- Partials     6757     6765       +8

Files with missing lines	Coverage Δ
src/env-inl.h	`97.06% <100.00%> (+0.22%)`	⬆️
src/env.h	`98.14% <ø> (ø)`
src/node_options.cc	`87.46% <100.00%> (+0.01%)`	⬆️
src/node_options.h	`98.27% <100.00%> (+<0.01%)`	⬆️
src/node_report.cc	`93.52% <100.00%> (+0.36%)`	⬆️
lib/internal/process/report.js	`98.57% <71.42%> (-1.43%)`	⬇️
src/node_report_module.cc	`90.00% <61.53%> (-2.22%)`	⬇️

... and 56 files with indirect coverage changes

jasnell · 2024-11-06T00:00:58Z

test/report/test-report-writereport-exclude-env.js

+}
+
+{
+  const error = new Error();


Nit: can you add a // Test with... type comment to this block as well?

legendecas

I think the PR title could be updated since the flag is renamed as "exclude" rather than "preserve".

legendecas · 2024-11-06T09:54:55Z

lib/internal/process/report.js

@@ -105,6 +107,12 @@ const report = {

    nr.setReportOnUncaughtException(trigger);
  },
+  get excludeEnv() {


Would you mind adding a setter like set reportOnSignal as well?

Done
fed0381

nodejs-github-bot · 2024-11-07T13:48:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63479/

nodejs-github-bot · 2024-11-08T13:38:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63490/

doc/api/cli.md

nodejs-github-bot · 2024-11-08T17:49:45Z

Landed in a243225

PR-URL: #55697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) #55626 PR-URL: TODO

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) #55626 PR-URL: #55921

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) nodejs#55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) nodejs#55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) nodejs#55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) nodejs#55626 PR-URL: nodejs#1

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) nodejs#55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) nodejs#55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) nodejs#55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) nodejs#55626 PR-URL: https://github.com/nodejs/aduh95/pull/1

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) nodejs#55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) nodejs#55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) nodejs#55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) nodejs#55626 PR-URL: #1

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) #55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) #55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) #55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) #55626 PR-URL: #55921

PR-URL: nodejs#55697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) nodejs#55732 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) nodejs#55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) nodejs#55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) nodejs#55626 PR-URL: nodejs#55921

Tofandel · 2024-11-21T16:29:17Z

Interestingly the changelog entry and commit message does not match the PR title
ccb69bb8d5
nodejs/nodejs.org@881afd4#diff-51e790f0f935b3734f4f20688ad920248a1412cabced30879384e74186e4480fR14

richardlau · 2024-11-21T17:12:13Z

Interestingly the changelog entry and commit message does not match the PR title ccb69bb8d5 nodejs/nodejs.org@881afd4#diff-51e790f0f935b3734f4f20688ad920248a1412cabced30879384e74186e4480fR14

That was due to the discussion in #55921 (comment).

Tofandel · 2024-11-21T17:34:35Z

No no, I mean in both the commit message and changelog they have "to preserve env" instead of "to exclude env" which is the old PR title

Notable changes: doc: * enforce strict policy to semver-major releases (Rafael Gonzaga) nodejs#55732 sqlite: * (SEMVER-MINOR) add support for SQLite Session Extension (Bart Louwers) nodejs#54181 src: * (SEMVER-MINOR) add cli option to preserve env vars on dr (Rafael Gonzaga) nodejs#55697 util: * (SEMVER-MINOR) add sourcemap support to getCallSites (Marco Ippolito) nodejs#55589 * (SEMVER-MINOR) fix util.getCallSites plurality (Chengzhong Wu) nodejs#55626 PR-URL: TODO

src: add cli option to preserve env vars on dr

ac4015d

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 2, 2024

anonrig reviewed Nov 2, 2024

View reviewed changes

lib/internal/process/report.js Outdated Show resolved Hide resolved

src/node_report_module.cc Outdated Show resolved Hide resolved

RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 2, 2024

RedYetiDev added the report Issues and PRs related to process.report. label Nov 3, 2024

fixup! src: add cli option to preserve env vars on dr

d4460ea

jasnell reviewed Nov 6, 2024

View reviewed changes

jasnell approved these changes Nov 6, 2024

View reviewed changes

legendecas reviewed Nov 6, 2024

View reviewed changes

RafaelGSS changed the title ~~src: add cli option to preserve env vars on dr~~ src: add cli option to exlcude env vars on dr Nov 6, 2024

RafaelGSS changed the title ~~src: add cli option to exlcude env vars on dr~~ src: add cli option to exclude env vars on dr Nov 6, 2024

fixup! fixup! src: add cli option to preserve env vars on dr

fed0381

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2024

richardlau approved these changes Nov 8, 2024

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2024

nodejs-github-bot merged commit a243225 into nodejs:main Nov 8, 2024
59 checks passed

RafaelGSS mentioned this pull request Nov 8, 2024

doc: sort --report-exclude alphabetically #55788

Merged

aduh95 pushed a commit that referenced this pull request Nov 16, 2024

src: add cli option to preserve env vars on dr

ccb69bb

PR-URL: #55697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

RafaelGSS mentioned this pull request Nov 19, 2024

2024-11-20, Version 23.3.0 (Current), @RafaelGSS #55921

Merged

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

src: add cli option to preserve env vars on dr

20498c4

PR-URL: nodejs#55697 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

RafaelGSS mentioned this pull request Nov 22, 2024

2024-11-29, Version 23.3.1 (Current), @RafaelGSS RafaelGSS/node#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add cli option to exclude env vars on dr #55697

src: add cli option to exclude env vars on dr #55697

RafaelGSS commented Nov 2, 2024 •

edited

Loading

richardlau commented Nov 2, 2024

RafaelGSS commented Nov 5, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

jasnell Nov 6, 2024

legendecas left a comment

legendecas Nov 6, 2024

RafaelGSS Nov 7, 2024

nodejs-github-bot commented Nov 7, 2024

nodejs-github-bot commented Nov 8, 2024

nodejs-github-bot commented Nov 8, 2024

Tofandel commented Nov 21, 2024 •

edited

Loading

richardlau commented Nov 21, 2024

Tofandel commented Nov 21, 2024 •

edited

Loading

src: add cli option to exclude env vars on dr #55697

src: add cli option to exclude env vars on dr #55697

Conversation

RafaelGSS commented Nov 2, 2024 • edited Loading

richardlau commented Nov 2, 2024

RafaelGSS commented Nov 5, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

jasnell Nov 6, 2024

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

legendecas Nov 6, 2024

Choose a reason for hiding this comment

RafaelGSS Nov 7, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 7, 2024

nodejs-github-bot commented Nov 8, 2024

nodejs-github-bot commented Nov 8, 2024

Tofandel commented Nov 21, 2024 • edited Loading

richardlau commented Nov 21, 2024

Tofandel commented Nov 21, 2024 • edited Loading

RafaelGSS commented Nov 2, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

Tofandel commented Nov 21, 2024 •

edited

Loading

Tofandel commented Nov 21, 2024 •

edited

Loading